Bugfix (8206) - Don't fail switch slot resolution when one switch zone is unresponsive by internet-diglett · Pull Request #8914 · oxidecomputer/omicron

internet-diglett · 2025-08-26T19:06:39Z

This PR addresses two issues that impacts users in circumstances where a switch zone is unavailable:

When resolving which switch slot is managed by which switch zone, we would continuously retry whenever there was a communication error. This causes RPWs to stop running / Sagas to get stuck whenever a switch zone becomes unavailable but still has entries in DNS.
In the dpd_ensure node of the instance_start saga, we were bailing out if we encountered any errors while notifying dpd of nat changes. This means that in the event of one of our switch zones being unavailable we would no longer allow users to start instances, which is probably a bit too strict.

This PR makes the following adjustments to the switch slot resolution behavior:

Log an error instead of a warning whenever a DNS entry exists for a switch zone service but communication fails
Return the mappings of the Slot -> Address for the switch zones that we were successfully able to resolve
Let the caller decide whether or not they should retry or proceed. After checking all of the current call sites, it appears that the logic already accommodates scenarios where switch zone mappings are missing.

This PR makes the following changes to the behavior to the NAT configuration steps for instance related sagas:

Do not bail if we fail to notify a dendrite service of NAT changes. Log a warning and rely on the NAT RPW to catch us back up whenever the switch zone becomes available again. Still return a Result from the notify function so that callers can decide if they want to make a subsequent decision based on the error.

This PR makes the following changes to the behavior of NAT garbage collection:

Pause GC (exit early) if we detect that both switches cannot be reached

Before now, due to a configuration issue, the instant start saga tests weren't actually driving ensuring that NAT configurations were eventually landing on the dendrite instances. It also turns out that Nexus in the TestContext could not actually communicate with dendrite because the port values were hard coded and our resolution logic relied on comparing the different addresses for MGS (and all mgs instances listen on localhost in testing). This PR introduces the following changes:

Use an updated version of Dendrite that allows us to provide a MGS SocketAddr on startup so it can use a simulated MGS to learn switch slot information
Adjust the TestContext Builder logic to provide MGS information to dendrite
Have Nexus ask dendrite for its switch slot information instead of having Nexus attempt to resolve it via MGS
Change the test configuration to create a NIC for the instance so we actually exercise the NAT creation logic in the dpd_ensure saga node
Introduce a test that ensures we still land configurations on the remaining switch even when one of our switches are unavailable

Depends On

oxidecomputer/dendrite#117

When resolving which switch slot is managed by which switch zone, we would continuously retry whenever there was a communication error. This causes RPWs to stop running / Sagas to get stuck whenever a switch zone becomes unavailable but still has entries in DNS. These changes have the following adjustment to the resolution behavior: * Log an error instead of a warning whenever a DNS entry exists for a switch zone service but communication fails * Return the mappings of the Slot -> Address for the switch zones that we were successfully able to resolve * Let the caller decided whether or not they should retry or proceed. After checking all of the current call sites, it appears that the logic already accomodates scenarios where switch zone mappings are missing.

Previously in `dpd_ensure` we were bailing out if we encounter any errors while notifying dpd of nat changes. This means that in the event of one of our switch zones being unavailable we would no longer allow users to start instances, which is probably a bit too strict. Also, since the dendrite daemons have a NAT reconciliation RPW, we can rely on it to catch us up instead of hard failing whenever there is a failure to notify them of updates.

hawkw · 2025-08-26T23:57:02Z

+    {
+        warn!(
+            log,
+            "error encountered when notifying dendrite, NAT entry creation may be delayed";


nit: mind wrapping this line?

I opted for a shorter log message since ultimately if someone is looking at this they're going to need to look at the code to get more context anyway.

Before now the instant start saga tests weren't actually driving ensuring that NAT configurations were eventually landing on the dendrite instances. It turns out that Nexus in the TestContext could not actually communicate with dendrite because the port values were hard coded and our resolution logic relied on comparing the different addresses for MGS (and all mgs instances listen on localhost in testing). This commit introduces the following changes: * Use an updated version of Dendrite that allows us to provide a MGS SocketAddr on startup so it can use a simulated MGS to learn switch slot information * Adjust the TestContext Builder logic to provide MGS information to dendrite * Have Nexus ask dendrite for its switch slot information instead of having Nexus attempt to resolve it via MGS * Change the test configuration to create a NIC for the instance so we actually exercise the NAT creation logic in the `dpd_ensure` saga node * Introduce a test that ensures we still land configurations on the remaning switch even when one of our switches are unavailable

rcgoodfellow

Thanks Levon. Mostly LGTM, just a few comments/questions.

rcgoodfellow · 2025-09-18T04:37:11Z

+    // We should not bail out if there is an error while notifying dendrite.
+    // If there is an error communicating with one dendrite instance but the
+    // other is operational and we bail here, it will prevent users from starting
+    // an instance.


I believe the idea here is that for any switches we cannot currently communicate with, the RPW will reconcile their state once they become available again? If that's correct can you add something to that effect in the comment?

Correct, and will do!

rcgoodfellow · 2025-09-18T04:47:09Z

+//
+/// Returns a mapping of clients for the LLDP daemons of reachable switch zones.
+/// If we are unable to communicate with the switch zone and determine the mapping
+/// of SwitchLocation -> Zone Underlay Address, we omit an entry for that client.


Why are we moving dpd_clients from using switch_zone_address_mappings which is using AAAA records to grabbing more specific SRV records but not lldpd_clients?

That is because dendrite is able to learn its switch slot directly from mgs while lldp does not (or maybe it does and I'm not aware). The plan is to update all of the switch daemons to learn their slot directly from their local mgs so we can move them to the same pattern that dpd_clients is using and no longer have Nexus resolve the switch slots. The motivation for this change is detailed in #8999, but TL;DR, anything that relies on hard coded port numbers in Nexus will break in the TestContext (which consequently means we do not have any tests that properly e2e test lldp, maghemite, etc. today).

rcgoodfellow · 2025-09-18T04:56:18Z

+/// We return whatever we're able to successfully resolve. In the event of a
+/// a communication timeout or other failure with MGS, the SwitchLocation -> Ipv6Addr
+/// mapping will be missing from the returned HashMap. Callers will need to inspect
+/// the contents to be ensure what they expect to be there is actually there.


to be ensure -> to ensure

rcgoodfellow · 2025-09-18T05:03:37Z

+            client_state,
+        );
+
+        // Check to ensure that the nat entry for the address has made it onto the switch


Are we able to unkill switch0 and assert that the RPW reconciles the NAT entry when it comes back online?

Good idea, I'll take a look!

I was able to get this working in the test

davepacheco

One thing I haven't checked yet is that this PR changes switch_zone_address_mappings() and dpd_clients() to potentially return fewer results instead of blocking indefinitely. That seems right directionally, but have you checked all the callers to make sure that they won't do something wrong if one of these is missing?

davepacheco · 2025-09-24T16:54:01Z

+        let mut uplink0_params = params::SwitchPortSettingsCreate::new(
+            IdentityMetadataCreateParams {
+                name: "test-uplink0".parse().unwrap(),
+                description: "test uplink".into(),
+            },
+        );
+
+        uplink0_params.routes = vec![params::RouteConfig {
+            link_name: "phy0".parse().unwrap(),
+            routes: vec![params::Route {
+                dst: "0.0.0.0/0".parse().unwrap(),
+                gw: "1.1.1.1".parse().unwrap(),
+                vid: None,
+                rib_priority: None,
+            }],
+        }];
+
+        let mut uplink1_params = params::SwitchPortSettingsCreate::new(
+            IdentityMetadataCreateParams {
+                name: "test-uplink1".parse().unwrap(),
+                description: "test uplink".into(),
+            },
+        );
+
+        uplink1_params.routes = vec![params::RouteConfig {
+            link_name: "phy0".parse().unwrap(),
+            routes: vec![params::Route {
+                dst: "0.0.0.0/0".parse().unwrap(),
+                gw: "2.2.2.2".parse().unwrap(),
+                vid: None,
+                rib_priority: None,
+            }],
+        }];
+
+        let uplink0_settings = datastore
+            .switch_port_settings_create(&opctx, &uplink0_params, None)
+            .await
+            .expect("should be able to create configuration for uplink0");
+
+        let uplink1_settings = datastore
+            .switch_port_settings_create(&opctx, &uplink1_params, None)
+            .await
+            .expect("should be able to create configuration for uplink1");
+
+        let rack_id = datastore
+            .rack_list(&opctx, &DataPageParams::max_page())
+            .await
+            .unwrap()
+            .pop()
+            .unwrap()
+            .identity
+            .id;
+
+        let uplink0 = datastore
+            .switch_port_get_id(
+                &opctx,
+                rack_id,
+                Name::try_from("switch0".to_string()).unwrap().into(),
+                Name::try_from("qsfp0".to_string()).unwrap().into(),
+            )
+            .await
+            .expect("there should be a switch port for switch0");
+
+        let uplink1 = datastore
+            .switch_port_get_id(
+                &opctx,
+                rack_id,
+                Name::try_from("switch1".to_string()).unwrap().into(),
+                Name::try_from("qsfp0".to_string()).unwrap().into(),
+            )
+            .await
+            .expect("there should be a switch port for switch1");
+
+        datastore
+            .switch_port_set_settings_id(
+                &opctx,
+                uplink0,
+                Some(uplink0_settings.settings.id()),
+                db::datastore::UpdatePrecondition::DontCare,
+            )
+            .await
+            .expect("unable to update switch0 settings");
+
+        datastore
+            .switch_port_set_settings_id(
+                &opctx,
+                uplink1,
+                Some(uplink1_settings.settings.id()),
+                db::datastore::UpdatePrecondition::DontCare,
+            )
+            .await
+            .expect("unable to update switch1 settings");


Sorry for my ignorance here but what's all this for (in the test)?

When the saga hits dpd_ensure, it only notifies dendrite instances that have a configured uplink (if the switch isn't connected to any external networks via an uplink, there is no reason to put NAT entries on them). Because of that, we need to configure uplinks in this test before we can check to see if NAT entries are populated on the switches.

It's probably worth adding a comment explaining what is happening here so I'll do that too

davepacheco · 2025-09-24T16:55:16Z

+        // Ensure that the nat entry for the address has made it onto the new switch0 dendrite.
+        // This might take a few seconds while the new dendrite comes online.
+        let mut nat_entries = vec![];
+        for _ in 0..5 {


We have a helper wait_for_condition for pattern.

I'd put the max bound higher than 5 seconds -- that's not much time on a system that's busy running lots of tests. Maybe 30 seconds?

davepacheco · 2025-09-24T16:56:15Z

    }

+    #[tokio::test]
+    async fn should_start_with_dead_switch() {


davepacheco · 2025-09-24T17:20:47Z

+    let sp_conf: Utf8PathBuf = DEFAULT_SP_SIM_CONFIG.into();
+    builder.start_gateway(SwitchLocation::Switch0, None, sp_conf.clone()).await;
+    builder.start_gateway(SwitchLocation::Switch1, None, sp_conf).await;


I saw this here and in another test. Is this because starting Nexus now depends on these things, or is this just because things don't always work in Nexus when these aren't running?

Do we have a test that Nexus still starts without these?

It's because dendrite needs MGS in order to work in the test suite (dendrite wasn't actually fully starting up in our test suite until this PR). I added these lines here because dendrite is started in the next few lines. I don't believe MGS being present is necessary for Nexus to start up.

davepacheco · 2025-09-24T17:25:02Z

As an example: I see that the sync_service_zone_nat background task uses switch_zone_address_mappings() to notify dpd if it noticed any changes. This might now only update one Dendrite. The next time around, it seems like result will be 0 and we won't notify the other Dendrite. Is this a problem?

davepacheco · 2025-09-24T17:40:32Z

I took a not-quite-exhaustive look through other callers. The callers of switch_zone_address_mappings() I'd be most worried about are:

sync_service_zone_nat.rs: mentioned above
nat_cleanup.rs: seems like it's assuming it can reach all of the switches -- it's trying to find the min generation deployed on any of them and cleaning up stuff that's older? What happens if a switch has an older version, but we miss it because we can't reach it, and then we clean it up?
sync_switch_configuration.rs: I'm not clear on this one

bfd.rs also uses it, but it seems fine because it seems to be always fetching everything and sync'ing everywhere, so a transient failure will be caught next time.

lldpd_clients() is used by lldp_neighbors_get(), in an external API "list" API. Seems like this might silently omit some data, which seems not great but maybe okay?

In terms of callers of dpd_clients():

probe_delete_dpd_config(), notify_dendrite_nat_state(), and probe_ensure_dpd_config() all look okay based on the comments there that a failure to notify dpd will be retried by the RPW
switch_port_status(): seems like this might silently omit stuff, which doesn't seem great, but maybe not a huge deal?

Sorry this was a bit of a cursory look and I'm not that familiar with these subsystems so take it all with a grain of salt!

Add a more detailed comment for test Co-authored-by: David Pacheco <dap@oxidecomputer.com>

internet-diglett · 2025-09-25T17:23:31Z

One thing I haven't checked yet is that this PR changes switch_zone_address_mappings() and dpd_clients() to potentially return fewer results instead of blocking indefinitely. That seems right directionally, but have you checked all the callers to make sure that they won't do something wrong if one of these is missing?

I did walk through all of the callers to verify that we wouldn't do something that wouldn't behave in an unexpected manner, and my initial take was that all of them were written to handle partial failures but they weren't actually ever allowed to handle the partial failure because we would get stuck in a loop inside of dpd_clients().

That said, having a second set of eyes go over this has been great, and I will take the time to re-review some of those concern points you called out in your subsequent comments to see if there are any gotchas that I didn't catch on my initial review of those areas.

internet-diglett · 2025-09-30T21:18:34Z

@davepacheco some updates for your questions:

I see that the sync_service_zone_nat background task uses switch_zone_address_mappings() to notify dpd if it noticed any changes. This might now only update one Dendrite. The next time around, it seems like result will be 0 and we won't notify the other Dendrite. Is this a problem?

This should not be a problem because the sync_service_zone_nat background task also updates the nat_entry db table, which dendrite's NAT RPW will catch up on even if it misses a notification from nexus.

sync_switch_configuration.rs: I'm not clear on this one

I walked through all of the logic here and we should be in the clear. It compares the state on each run and reconciles whatever is different, so if dendrite isn't available on a given run and comes back for the next run, the sync_switch_configuration background task should successfully get it caught up again.

nat_cleanup.rs: seems like it's assuming it can reach all of the switches -- it's trying to find the min generation deployed on any of them and cleaning up stuff that's older? What happens if a switch has an older version, but we miss it because we can't reach it, and then we clean it up?

I've been thinking about this for several days. I think for the near term, it should be okay if we ~~pause~~ exit this task early if a switch is missing, effectively pausing this task until the switch comes back. In the long term, I think this will need to be redesigned, especially once we enter the multi-rack world.

davepacheco

Just some very small suggestions here.

davepacheco · 2025-10-07T19:01:15Z

+                let dpd_client = dpd_client::Client::new(
+                    &format!("http://[{addr}]:{port}"),
+                    client_state.clone(),
+                );


This probably doesn't matter much being a test, but you could clone the client that we already had (or even wrap it in an Arc).

davepacheco · 2025-10-07T19:02:00Z

+                        .await
+                    {
+                        Ok(v) => Ok(v),
+                        // sometimes we reach this point before the dendrite API is available


nit: I'd suggest logging something for this case.

The pattern I like with wait_for_condition when used like this would be to always log result above. That's an easy way to have it in both success and error cases.

davepacheco · 2025-10-07T19:03:20Z

+                    match result.items.is_empty() {
+                        // if the nat entries are empty the dendrite NAT RPW hasn't
+                        // successfully reconciled yet
+                        true => Err(poll::CondCheckError::<()>::NotYet),
+                        false => Ok(()),
+                    }


Suggested change

match result.items.is_empty() {

// if the nat entries are empty the dendrite NAT RPW hasn't

// successfully reconciled yet

true => Err(poll::CondCheckError::<()>::NotYet),

false => Ok(()),

}

if result.items.is_empty() {

// if the nat entries are empty the dendrite NAT RPW hasn't

// successfully reconciled yet

Err(poll::CondCheckError::<()>::NotYet)

} else {

Ok(())

}

(was there a reason this was a match?)

(was there a reason this was a match?)

Not really. When I was iterating on it I think I was matching on a different value and refactored that code to use is_empty() and didn't bother changing it to a simple if after the fact. 😂

davepacheco · 2025-10-07T19:03:58Z

+        // We only eagerly populate NAT entries on switches that have uplinks configured, so we need to
+        // do some switch configuration before starting the instance in order to test that section of logic


nit: these lines are over 80 columns (just that rustfmt's option for doing this automatically is unstable)

…0389) Currently, the `instance_update` saga will unwind if it attempts to update an instance whose active VMM has transitioned to `Destroyed` or `Failed` while one switch zone is unreachable. This means that the instance will remain in the `Stopping` state, and will not transition to `Stopped` or `Failed`, until both switch zones have come back. This is unfortunate, since it prevents the instance from being deleted or restarted, either manually by a user or automatically by instance reincarnation, until the disappeared switch reappears. This is described in #10343. This commit fixes this issue by changing the `instance_networking::instance_delete_dpd_config` function to return `Ok(())` even if an error occurs while notifying dendrite of the NAT state change. This is safe to do because the latest NAT state is propagated to both switches by an RPW, so when the switch that has disappeared returns, it will still receive the latest state even if we were not able to notify it in the saga. This is identical to what `instance_ensure_dpd_config` has done since PR #8914, and we just needed to make the same change in `instance_delete_dpd_config`. I've also added a test ensuring that the instance update saga can complete while one Dendrite is gone, which failed prior to this change. Fixes #10343.

internet-diglett added 2 commits August 26, 2025 18:03

hawkw reviewed Aug 26, 2025

View reviewed changes

internet-diglett added 5 commits September 5, 2025 22:29

nit / test fixes

4cf65c7

Merge branch 'main' into bugfix-8206

69d21b7

fixup! Merge branch 'main' into bugfix-8206

73bbafb

fixup! Merge branch 'main' into bugfix-8206

18a3c52

internet-diglett changed the title ~~Bugfix 8206~~ Bugfix (8206) - Don't fail switch slot resolution when one switch zone is unresponsive Sep 15, 2025

internet-diglett requested a review from davepacheco September 17, 2025 21:37

rcgoodfellow reviewed Sep 18, 2025

View reviewed changes

pr review fixes

31e475a

rcgoodfellow approved these changes Sep 24, 2025

View reviewed changes

davepacheco reviewed Sep 24, 2025

View reviewed changes

Update nexus/src/app/sagas/instance_start.rs

644aed0

Add a more detailed comment for test Co-authored-by: David Pacheco <dap@oxidecomputer.com>

morlandi7 added this to the 17 milestone Sep 25, 2025

morlandi7 assigned internet-diglett Sep 25, 2025

internet-diglett mentioned this pull request Sep 30, 2025

remove listen_addr requirement for rpw startup oxidecomputer/dendrite#117

Merged

internet-diglett added 3 commits September 30, 2025 20:29

add more comments to clarify purpose of testing logic

779af95

use wait_for_condition in test

7599335

add comment explaining mgs configuration logic

6ddc7f6

internet-diglett added 3 commits October 1, 2025 23:59

return early from nat cleanup if both switches are not available

b0b78c5

give dendrite a bit of time to come up for test

48bf721

Merge branch 'main' into bugfix-8206

c9c0126

davepacheco approved these changes Oct 7, 2025

View reviewed changes

more pr touchups and cleanup

7c53622

internet-diglett enabled auto-merge (squash) October 8, 2025 03:04

internet-diglett merged commit ca91632 into main Oct 8, 2025

internet-diglett deleted the bugfix-8206 branch October 8, 2025 03:20

This was referenced Apr 29, 2026

[nexus] fail instances if the sled they're on isn't...on #10334

Merged

instance_update sagas cannot complete while one switch zone is down #10343

Closed

hawkw mentioned this pull request May 6, 2026

nexus: allow instance updates to complete when one switch is down #10389

Merged

		// We only eagerly populate NAT entries on switches that have uplinks configured, so we need to
		// do some switch configuration before starting the instance in order to test that section of logic

Conversation

internet-diglett commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related

Depends On

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcgoodfellow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcgoodfellow Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcgoodfellow Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

internet-diglett Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Sep 24, 2025

Uh oh!

davepacheco commented Sep 24, 2025

Uh oh!

internet-diglett commented Sep 25, 2025

Uh oh!

internet-diglett commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

internet-diglett commented Aug 26, 2025 •

edited

Loading

rcgoodfellow Sep 18, 2025 •

edited

Loading

rcgoodfellow Sep 18, 2025 •

edited

Loading

internet-diglett Sep 25, 2025 •

edited

Loading

internet-diglett commented Sep 30, 2025 •

edited

Loading